Skip to content

My Code Review suggestions#2

Open
trentaml wants to merge 1 commit intomasterfrom
trentaml_CodeReview1
Open

My Code Review suggestions#2
trentaml wants to merge 1 commit intomasterfrom
trentaml_CodeReview1

Conversation

@trentaml
Copy link
Copy Markdown
Collaborator

@trentaml trentaml commented Mar 13, 2020

Project Description:
This is a program/app that collects user music suggestions for a party and curates a playlist for the party based around the user suggestions as to satisfy all party-goers with different musical tastes

What I learned:
*Learned proper naming conventions for packages
*Learned how to efficiently rename multiple packages at once if necessary

Finding changes to make were a little difficult as there was not much code readily available

@@ -1,4 +1,4 @@
package com.example.party_player
package com.example.partyPlayer
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the package names by removing underscores to comply with Kotlin naming conventions stated here: https://kotlinlang.org/docs/reference/coding-conventions.html

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While they are two words, package names are usually all lowercase. Consider making this all lowercase.

<?xml version="1.0" encoding="utf-8"?>
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
package="com.example.party_player">
package="com.example.partyPlayer">
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the package names by removing underscores to comply with Kotlin naming conventions stated here: https://kotlinlang.org/docs/reference/coding-conventions.html

*/
@RunWith(AndroidJUnit4::class)
class ExampleInstrumentedTest {
class InstrumentedTest {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the name of the test files to be a little more visibly unique. (Both test files began with "Example")

* See [testing documentation](http://d.android.com/tools/testing).
*/
class ExampleUnitTest {
class UnitTest {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the name of the test files to be a little more visibly unique. (Both test files began with "Example")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're removing the "Example" prefix, rename the test to something that is more descriptive of what is being tested. "UnitTest" is implied here, based on the location of this class. What is being unit tested? Put that in the name of the class.


/**
* Example local unit test, which will execute on the development machine (host).
*
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaned up the comments by removing blank gaps.


/**
* Instrumented test, which will execute on an Android device.
*
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaned up the comments by removing blank gaps.

android:supportsRtl="true"
android:theme="@style/AppTheme">
<activity android:name=".MainActivity">
<activity android:name="com.example.partyPlayer.MainActivity">
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the package names by removing underscores to comply with Kotlin naming conventions stated here: https://kotlinlang.org/docs/reference/coding-conventions.html

@@ -1,4 +1,4 @@
package com.example.party_player
package com.example.partyPlayer
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the package names by removing underscores to comply with Kotlin naming conventions stated here: https://kotlinlang.org/docs/reference/coding-conventions.html

@@ -1,15 +1,14 @@
package com.example.party_player
package com.example.partyPlayer
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the package names by removing underscores to comply with Kotlin naming conventions stated here: https://kotlinlang.org/docs/reference/coding-conventions.html

jabennett1515 added a commit to jabennett1515/Party-Player that referenced this pull request Apr 6, 2020
@discospiff
Copy link
Copy Markdown

I'll leave it up to the team to decide whether or not to merge. See inline commentary for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants